client: ignore http status header for gRPC streams #8548
Conversation
Fixes grpc#8486 When a gRPC response is received with content type application/grpc, we then do not expect any information in the http status and the status information needs to be conveyed by gRPC status only. In case of missing gRPC status, we will throw an Internal error instead of Unknown in accordance with https://grpc.io/docs/guides/status-codes/ Changes : - Ignore http status in case of content type application/grpc - Change the default rawStatusCode to return Internal for missing grpc status RELEASE NOTES: * client : Ignore http status for gRPC mode and return Internal error for missing grpc status
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #8548 +/- ##
==========================================
- Coverage 81.96% 81.90% -0.06%
==========================================
Files 415 415
Lines 40694 40690 -4
==========================================
- Hits 33355 33328 -27
- Misses 5950 5985 +35
+ Partials 1389 1377 -12
🚀 New features to boost your workflow:
|
|
In the grpc-go repo, we use the assignee to indicate who the review is pending on (author or reviewer). We ignore the reviewer status (the yellow pending re-review dot). Assigning to myself. |
|
For the PR title, we add the name of the Go package (e.g. |
mbissa
force-pushed
the
ignore-status-for-grpc-content-type-8486
branch
2 times, most recently
from
e1f6dbc to
f123124
Compare
…rpc-content-type-8486
mbissa
force-pushed
the
ignore-status-for-grpc-content-type-8486
branch
from
f123124 to
b4cce55
Compare
arjan-bal
changed the title
mbissa
changed the title
mbissa
changed the title
internal/transport/http2_client.go
Outdated
| // headerError is set if an error is encountered while parsing the headers | ||
| headerError string | ||
| headerError string | ||
| receivedHTTPStatus string |
There was a problem hiding this comment.
nit: We can omit the received prefix int the variable name, shortening it to httpStatus since it doesn't effect readability.
internal/transport/http2_client.go
Outdated
| case "200": | ||
| grpcErrorCode = codes.Unknown |
There was a problem hiding this comment.
Is this explicit check required? Since 200 isn't present in the HTTPStatusConvTab map, I think it may be omitted.
There was a problem hiding this comment.
The idea is when no error information can be derived from http status since it is 200, we can simply return Unknown code.
There was a problem hiding this comment.
The default case also adds a error message which seems useful. I think we should remove the special case unless it requires different handling in code. We don't need to worry too much about optimizing the error case, since they're expected to be infrequent.
There was a problem hiding this comment.
+1; please remove special cases when possible most of the time. Less code = easier to understand code.
arjan-bal
changed the title
arjan-bal
changed the title
internal/transport/http2_client.go
Outdated
| case "200": | ||
| grpcErrorCode = codes.Unknown |
There was a problem hiding this comment.
The default case also adds a error message which seems useful. I think we should remove the special case unless it requires different handling in code. We don't need to worry too much about optimizing the error case, since they're expected to be infrequent.
internal/transport/http2_client.go
Outdated
| // headerError is set if an error is encountered while parsing the headers | ||
| headerError string | ||
| headerError string | ||
| receivedHTTPStatus string |
internal/transport/http2_client.go
Outdated
| // headerError is set if an error is encountered while parsing the headers | ||
| headerError string | ||
| headerError string | ||
| receivedHTTPStatus string |
internal/transport/http2_client.go
Outdated
| case "200": | ||
| grpcErrorCode = codes.Unknown |
There was a problem hiding this comment.
+1; please remove special cases when possible most of the time. Less code = easier to understand code.
mbissa
force-pushed
the
ignore-status-for-grpc-content-type-8486
branch
from
9c920c8 to
734cd4d
Compare
mbissa
force-pushed
the
ignore-status-for-grpc-content-type-8486
branch
from
a4c1366 to
09cf708
Compare
|
|
||
| test.metaHeaderFrame.HeadersFrame = &http2.HeadersFrame{ | ||
| // TestClientDecodeTrailer validates the handling of trailer frames, which may | ||
| // or may not also be the initial header frame (header-only response). |
There was a problem hiding this comment.
| // or may not also be the initial header frame (header-only response). | |
| // or may not also be the initial header frame (trailers-only response). |
Terminology from https://github.com/grpc/grpc/blob/master/doc/PROTOCOL-HTTP2.md#responses
| // Mark headerChanClosed to indicate trailer frames. | ||
| cs.headerChanClosed = 1 | ||
| // Simulate the state where the initial headers have already been processed. |
There was a problem hiding this comment.
Both of these comments seem like they're related to the same thing, which is setting headerChanClosed.
| }{ | ||
| { | ||
| name: "missing gRPC status", | ||
| name: "missing gRPC content type", |
There was a problem hiding this comment.
It seems like you have changed this test instead of adding a new one? Shouldn't we still have a test that sets these exact header fields?
There was a problem hiding this comment.
missing grpc status is no longer relevant in the the initial headers - it is tested in the trailers test where grpc status is expected/mandatory.
| }, | ||
| { | ||
| name: "malformed grpc-status-details-bin field with status 200", | ||
| name: "malformed grpc-status-details-bin field with status 404 and no content type", |
There was a problem hiding this comment.
Similar - don't we still want to have a test with an illegal grpc-status-details-bin and status 200? We can also have this new test case, but it seems like having more test cases is always better.
There was a problem hiding this comment.
If the content type is application/grpc (as was in previous version of the test) the http status is immaterial (200/404 etc won't make a difference) and it is tested in the test case above this with 404. This was more important variation of data where we check without correct content type.
Fixes #8486
When a gRPC response is received with content type application/grpc, we then do not expect any information in the http status and the status information needs to be conveyed by gRPC status only.
In case of missing gRPC status, we will throw an Internal error instead of Unknown in accordance with https://grpc.io/docs/guides/status-codes/
RELEASE NOTES: